-
Notifications
You must be signed in to change notification settings - Fork 257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tweaks to new Wrapperless mode #204
Conversation
Some small tweaks to @tobilen's excellent work creating a wrapperless mode for React Flip Move with React 16. - Removed the `anchor`, as this will be computed based on the parent - Removed the warning for not supplying an anchor - Update docs - Fix bug with the first transition being buggy, by setting the parentData on mount - Extract `findDOMContainer` method
* [Contributions](#contributions) | ||
* [Development](#development) | ||
* [Flow support](#flow-support) | ||
* [License](#license) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TOC was quite outdated, and I realized it'll always be problematic. This doc isn't so long now that API Reference
is its own thing, so I ditched the TOC.
Wrapperless mode is nice, because it makes FlipMove more "invisible", and makes it easier to integrate with parent-child CSS properties like flexbox. However, there are some things to note: | ||
|
||
- This is a new feature in FlipMove, and isn't as battle-tested as the traditional method. Please test thoroughly before using in production, and report any bugs! | ||
- FlipMove does some positioning magic for enter/exit animations - specifically, it temporarily applies `position: absolute` to its children. For this to work correctly, you'll need to make sure that `<FlipMove>` is within a container that has a non-static position (eg. `position: relative`), and no padding: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I noticed in Storybook; It's kind of a bummer :( and makes me think that wrapperless mode will likely cause more trouble than it's worth... but I figure we'll see. If we get a ton of confusing issues, maybe we can add a check that console.warns when the parent DOM element isn't configured correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we already have the dom node, those styles could be overwritten after findDOMNode
:
findDOMContainer = () => {
// eslint-disable-next-line react/no-find-dom-node
const domNode = ReactDOM.findDOMNode(this);
const parentNode = domNode && domNode.parentNode;
// This ought to be impossible, but handling it for Flow's sake.
if (!parentNode || !(parentNode instanceof HTMLElement)) {
return;
}
// Make sure parentNode has position relative and no padding
parentNode.style.position = 'relative';
parentNode.style.padding = 0;
this.parentData.domNode = parentNode;
};
should be mentioned in the docs though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parentNode.style.position = 'relative';
We can do that only if window.getComputedStyle(parentNode) === 'static')
And we can't change the padding as it would break the layout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so I thought about applying a "default" relative position if it isn't set to something other than static
, but it feels a bit like it crosses a line; we'd be mutating nodes outside of Flip Move's control. It could cause confusion for users if they have absolutely-positioned things down-tree.
So either we apply position: relative, which fixes FlipMove but breaks their layout, or we do nothing, and it breaks enter/exit animations. The latter is clearly a FlipMove issue, and so they'll come here and we can advise them... but the former might appear to them as a CSS issue / they won't understand why things are broken, why a random position: relative
is being applied to their div
...
Maybe we can apply relative positioning if it's static, and also console.warn
to let them know what's happening? But, if the user doesn't need enter/leave animations, they don't need this position: relative
, and so the warning would just be console spam.
That said, it's really rare that position: static
is actually desirable (I've seen a trend where people just add a * { position: relative }
global CSS rule), so I think it's probably best to optimize for the common usecase.
Gonna do it in a subsequent PR to make reviews easier, but I'll apply @Hypnosphi's static check, and issue a console.warn when the change is necessary.
| `String` | 'div' | | ||
| **Accepted Types:** | **Default Value** | | ||
|----------------------|-------------------| | ||
| `String` | `null` | 'div' | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this in #202, and didn't see any strong opinions; my preference is that we use null
over false
, since it feels more "pure" semantically.
If y'all think there's value in also supporting false
(or if there's an argument I haven't thought of for why false
makes more sense?), happy to update :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer null
@@ -1,23 +1,23 @@ | |||
declare module "react-flip-move" { | |||
declare module 'react-flip-move' { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, Prettier updated all the formatting here, adding trailing commas and single-quotes.
@Hypnosphi does this matter?
Also, I renamed this file from 2.9
to 2.10
, as the version will change. Should there be a flow-typed
file for every version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, pretty sure there should be. we don't want to allow null as a value on versions lower than 2.10.x
you can use wildcards in the name to span multiple versions though, like we're currently doing with the patch version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this matter?
If it doesn't for you, you shouldn't use prettier =) And I believe, trailing commas are one of the few things that are configurable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, pretty sure there should be. we don't want to allow null as a value on versions lower than 2.10.x
Yep, makes sense, thanks!
If it doesn't for you, you shouldn't use prettier =) And I believe, trailing commas are one of the few things that are configurable
You misunderstand; I was asking specifically about flow-typed
. Wanted to make sure there wasn't a different community convention for these type files (or, worse, third-party tools that don't understand trailing commas)
But yeah, sounds like it's a non-issue, so I'll go ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we kinda introduce new API, so it requires an updated libdef
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think we should disable eslint for flow-typed directory so that it’s easier to sync it with the version in flow-typed repo
Some small tweaks to @tobilen's excellent work creating a
wrapperless mode for React Flip Move with React 16.
anchor
, as this will be computed based on the parentfindDOMContainer
methodfalse
tonull
fortypeName